KV v2 logging improvements #45418
Merged
KV v2 logging improvements #45418
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lmolkova
requested review from
joshfree,
srnagar,
anuchandy,
conniey and
JonathanGiles
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on improving logging practices across the KV v2 codebase by replacing ad hoc try‐catch blocks and manual exception constructions with a consistent usage of key‐value logging and utility methods. The key changes include using LOGGER.throwableAtError().log(...) in place of LOGGER.logThrowableAsError() and adopting Objects.requireNonNull for null checks.
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sdk/keyvault-v2/azure-security-keyvault-keys/.../Aes128Kw.java | Updated key validation to pass LOGGER for consistent error logging. |
| sdk/keyvault-v2/azure-security-keyvault-keys/.../CryptographyClientBuilder.java | Replaced try-catch error wrapping with direct LOGGER.throwableAtError() calls. |
| sdk/keyvault-v2/azure-security-keyvault-keys/.../KeyClientBuilder.java | Replaced manual null-checks with Objects.requireNonNull. |
| (Numerous certificate and administration files) | Standardized logging exceptions and null checks, removed redundant try-catch blocks and improved error messages with key-value metadata. |
Comments suppressed due to low confidence (3)
sdk/keyvault-v2/azure-security-keyvault-keys/src/main/java/com/azure/v2/security/keyvault/keys/cryptography/CryptographyClientBuilder.java:151
- Verify that replacing the catch-log-rethrow pattern with direct logging using throwableAtError() produces the intended exception type and message consistency across the API.
throw LOGGER.throwableAtError().log("Key caching cannot be disabled when using a JSON Web Key.", IllegalStateException::new);
sdk/keyvault-v2/azure-security-keyvault-administration/src/main/java/com/azure/v2/security/keyvault/administration/KeyVaultAdministrationUtil.java:192
- [nitpick] Consider adding more contextual information (such as method or parameter names) to the log message to help diagnose unsupported operation types more easily.
throw logger.throwableAtError().addKeyValue("operationType", operation == null ? null : operation.getClass().getCanonicalName()).log(UnsupportedOperationException::new);
sdk/keyvault-v2/azure-security-keyvault-keys/src/main/java/com/azure/v2/security/keyvault/keys/cryptography/implementation/Aes128Kw.java:29
- Ensure that passing the LOGGER instance to the validation method is safe and that the method properly handles a null logger if it could occur in other contexts.
CryptographyUtils.validate(key, KEY_SIZE_IN_BYTES, LOGGER);
lmolkova
commented
May 21, 2025
...src/main/java/com/azure/v2/security/keyvault/administration/KeyVaultAccessControlClient.java
Show resolved
Hide resolved
vcolin7
reviewed
May 27, 2025
...src/main/java/com/azure/v2/security/keyvault/administration/KeyVaultAccessControlClient.java
Show resolved
Hide resolved
...n/java/com/azure/v2/security/keyvault/administration/KeyVaultAccessControlClientBuilder.java
Show resolved
Hide resolved
...src/main/java/com/azure/v2/security/keyvault/administration/KeyVaultBackupClientBuilder.java
Outdated
Show resolved
Hide resolved
...ava/com/azure/v2/security/keyvault/certificates/implementation/KeyVaultCredentialPolicy.java
Show resolved
Hide resolved
...a/com/azure/v2/security/keyvault/administration/implementation/KeyVaultCredentialPolicy.java
Show resolved
Hide resolved
...om/azure/v2/security/keyvault/keys/cryptography/implementation/Asn1DerSignatureEncoding.java
Show resolved
Hide resolved
...om/azure/v2/security/keyvault/keys/cryptography/implementation/RsaKeyCryptographyClient.java
Show resolved
Hide resolved
.../java/com/azure/v2/security/keyvault/keys/cryptography/implementation/SignatureEncoding.java
Show resolved
Hide resolved
...urity-keyvault-keys/src/main/java/com/azure/v2/security/keyvault/keys/models/JsonWebKey.java
Outdated
Show resolved
Hide resolved
...ava/com/azure/v2/security/keyvault/certificates/implementation/KeyVaultCredentialPolicy.java
Show resolved
Hide resolved
vcolin7
approved these changes
May 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Logging + exceptions improvements: